Skip to content

Added getters for JsonPatch and Operations #79

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 6, 2020
Merged

Added getters for JsonPatch and Operations #79

merged 2 commits into from
Aug 6, 2020

Conversation

GavinF17
Copy link
Contributor

Hoping this gets picked up & released due to recent activity by @Capstan :)

  • Added JsonPatch.getOperations()
  • Added field getters for operations
    • DualPathOperation.getFrom()
    • JsonPatchOperation.getOp() and JsonPatchOperation.getPath()
    • PathValueOperation.getValue()

Personal use case is needing to determine which class to patch when using subtypes, and the type info being held in a field which may be modified by a replace operation.

Solves #33
Supercedes #45

@Spellmaker
Copy link

I'm also highly interested in this PR getting merged, getters would be really useful for examining the operations (in my use-case: checking for certain paths to discourage changes to some part of the json document)

@serac
Copy link

serac commented Jul 28, 2020

My use case is similar to @Spellmaker but notably different. We have JSON models that map to database models where only some fields are mutable via our REST API. We need to be able to examine a patch request in order to validate that the JSON paths are mutable according to business policy. While we could do this with raw JSON and then parse it into a JsonPatch object after validation, it would be far more convenient to work with the parsed version where the operation and path are easily accessible on a JsonPatch object.

As far as this PR, it seems innocuous as operations is final and wrapped in an immutable collection, so there's no risk of clients mutating the internal state by exposing it via a getter. I hope you'll consider this enhancement.

Copy link
Contributor

@Capstan Capstan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall.

@@ -94,6 +94,14 @@ protected JsonPatchOperation(final String op, final JsonPointer path)
public abstract JsonNode apply(final JsonNode node)
throws JsonPatchException;

public String getOp() {
return op;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this protectively copy the string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't have thought this is necessary as String is immutable, if I'm not thinking of something can you please explain?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you're right.

@GavinF17
Copy link
Contributor Author

GavinF17 commented Aug 5, 2020

Looks good overall.

Thanks for the feedback, addressed all comments with one outstanding

@Capstan Capstan merged commit f8a4409 into java-json-tools:master Aug 6, 2020
@elalex2021
Copy link

elalex2021 commented Jul 26, 2021

I am using v.13 (https://mvnrepository.com/artifact/com.github.java-json-tools/json-patch/1.13), but still not able to use method getOperations().

image

@ceenlrnjim
Copy link

We are also interested in having the getOperations accessor and see that it isn't in 1.13 - are there plans for another release that will pick up this change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants